Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MQTT.js vNext: Initial Spike #1318

Merged
merged 2 commits into from
Sep 21, 2021
Merged

MQTT.js vNext: Initial Spike #1318

merged 2 commits into from
Sep 21, 2021

Conversation

YoDaMa
Copy link
Contributor

@YoDaMa YoDaMa commented Aug 19, 2021

Hi MQTT.js community,

Since joining this project and working on-and-off on maintenance tasks and github issues, I've slowly built up an understanding of the key strengths and weaknesses I've felt in the architecture of MQTT.js. I've released a major update along with various minor and patch version updates to fix security and usability bugs in the existing codebase. I've spent a significant amount of time exploring and redesigning the highly-flakey gate, migrating away from Travis to Github Actions, improving test isolation, and increasing overall reliability of the test code.

After contributing in this way I feel we've reached a point as a developer community where we should embark on a significant re-architecture of the package. This is the purpose of this PR.

What you'll see right now is an initial spike of a new code architecture for MQTT.js that I believe will improve readability and code maintainability. One of my goals here is to modernize the codebase toward ES6, and leverage rollup for cross platform (particularly with Node.js) usability. As these platforms modernize their engines to match ES6, rollup will be able to fill in the gaps.

Another goal in this spike is a more "handler" oriented design, similar to moscajs's aedes package.

There are many blindspots in my design and understanding, which is why I'd like community input, particularly from other longtime maintainers, on this.

I'd also like to understand the main benefits people might see in Paho MQTT, and how those benefits could be incorporated into this package.

So in conclusion, a re-design is long overdue, and necessary to keep this package up-to-date and functioning as a cornerstone MQTT Client package in JavaScript. I can lead this effort of course, but the more community input and knowledge-share, the better.

@mcollina
Copy link
Member

I really like this design. I also like you are aiming for a full rewrite and that you are targeting esm: +1.

(I won't be opposed for this to be in TS either - you'd be doing the vast majority of this anyway).

I recommend to throw away all the tests.

@YoDaMa
Copy link
Contributor Author

YoDaMa commented Aug 25, 2021

Here's my rough visualization of the data flow of a "connect" call in the current MQTT.js architecture.

image

My thought is that work and workNextTick seem like unnecessary complications considering the event driven nature of the streams. Simplifying the data flow would look approximately like this:

image

@YoDaMa
Copy link
Contributor Author

YoDaMa commented Aug 25, 2021

Also here is a rough diagram of the method flow when someone would call a "connect" in the current SDK.
image

It's very convoluted, particularly because setupStream is such a misnaming of the functionality of that piece of code. Additionally there's a really confusing back and forth from the index.js file in connect folder, where the connect method will create a wrapper function around a transport layer implementation that will then be passed as a function to the client for instantiation where the client calls the wrapper function in setupStream while also instantiating mqtt-packet's parser, actually building the transport stream (aka calling wrapper which is passed in as streamBuilder when instantiating the client), performing the necessary wiring to connect the different streams together so that the writable stream inside the client can process data by storing it and then calling a work function (which is all part of the setup but not really relevant in terms of data flow right now), and then it also finally sends a connect packet, while also creating a timeout for a connack.

In my view.... there's got to be a much better way :)

@mcollina
Copy link
Member

Sounds good!

@YoDaMa YoDaMa changed the title MQTT.js v5: Initial Spike MQTT.js vNext: Initial Spike Aug 26, 2021
@YoDaMa YoDaMa changed the base branch from master to vNext August 26, 2021 17:13
@YoDaMa YoDaMa merged commit 8e71d01 into vNext Sep 21, 2021
@YoDaMa YoDaMa deleted the yodama/redesign branch September 30, 2021 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants